-
-
Notifications
You must be signed in to change notification settings - Fork 453
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fill QuerySet generics using the manager's model type #2281
Conversation
reveal_type(MyModel.objects_1.all()) # N: Revealed type is "myapp.models.ModelQuerySet[myapp.models.MyModel]" | ||
reveal_type(MyModel.objects_2.all()) # N: Revealed type is "myapp.models.ModelQuerySet[myapp.models.MyModel]" | ||
reveal_type(MyModel.objects_1.all()) # N: Revealed type is "myapp.models.ModelQuerySet" | ||
reveal_type(MyModel.objects_2.all()) # N: Revealed type is "myapp.models.ModelQuerySet" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these (and all the similar ones) are more correct now -- these classes aren't generic so they shouldn't have type variables
I'm actually surprised mypy's internals don't enforce that an Instance
has the correct number of typevars filled in!
@@ -280,7 +285,7 @@ | |||
objects = MyModelQuerySet.as_manager() | |||
|
|||
class MyOtherModel(models.Model): | |||
objects = _MyModelQuerySet2.as_manager() # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, this was previously:
E myapp/models:35: error: Need type annotation for "objects" [var-annotated] (diff)
3b658fa
to
658de86
Compare
actually I have a better idea I want to try out for the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! I was fiddling around with those Var
insertions in #2280 but it would've taken me ages to figure out to swap to a method hook.
actually I have a better idea I want to try out for the as_manager stuff -- so hold off on merging for a bit :)
Just let me know when you want me to have a look again
tried instead overriding the base class hook to add the diff --git a/mypy_django_plugin/main.py b/mypy_django_plugin/main.py
index 55455037..e9197324 100644
--- a/mypy_django_plugin/main.py
+++ b/mypy_django_plugin/main.py
@@ -37,8 +37,7 @@ from mypy_django_plugin.transformers import (
)
from mypy_django_plugin.transformers.functional import resolve_str_promise_attribute
from mypy_django_plugin.transformers.managers import (
- construct_as_manager_instance,
- create_new_manager_class_from_as_manager_method,
+ add_as_manager_to_queryset_class,
create_new_manager_class_from_from_queryset_method,
reparametrize_any_manager_hook,
resolve_manager_method,
@@ -209,10 +208,6 @@ class NewSemanalDjangoPlugin(Plugin):
fullnames.REVERSE_MANY_TO_ONE_DESCRIPTOR: manytoone.refine_many_to_one_related_manager,
}
return hooks.get(class_fullname)
- elif method_name == "as_manager":
- info = self._get_typeinfo_or_none(class_fullname)
- if info and info.has_base(fullnames.QUERYSET_CLASS_FULLNAME):
- return partial(construct_as_manager_instance, info=info)
if method_name in self.manager_and_queryset_method_hooks:
info = self._get_typeinfo_or_none(class_fullname)
@@ -250,6 +245,11 @@ class NewSemanalDjangoPlugin(Plugin):
# Base class is a Form class definition
if fullname in self._get_current_form_bases():
return transform_form_class
+
+ # Base class is a QuerySet class definition
+ sym = self.lookup_fully_qualified(fullname)
+ if sym is not None and isinstance(sym.node, TypeInfo) and sym.node.has_base(fullnames.QUERYSET_CLASS_FULLNAME):
+ return add_as_manager_to_queryset_class
return None
def get_attribute_hook(self, fullname: str) -> Optional[Callable[[AttributeContext], MypyType]]:
@@ -308,10 +308,6 @@ class NewSemanalDjangoPlugin(Plugin):
info = self._get_typeinfo_or_none(class_name)
if info and info.has_base(fullnames.BASE_MANAGER_CLASS_FULLNAME):
return create_new_manager_class_from_from_queryset_method
- elif method_name == "as_manager":
- info = self._get_typeinfo_or_none(class_name)
- if info and info.has_base(fullnames.QUERYSET_CLASS_FULLNAME):
- return create_new_manager_class_from_as_manager_method
return None
def report_config_data(self, ctx: ReportConfigContext) -> Dict[str, Any]:
diff --git a/mypy_django_plugin/transformers/managers.py b/mypy_django_plugin/transformers/managers.py
index ec13bf20..9404b5f8 100644
--- a/mypy_django_plugin/transformers/managers.py
+++ b/mypy_django_plugin/transformers/managers.py
@@ -16,7 +16,8 @@ from mypy.nodes import (
SymbolTableNode,
TypeInfo,
)
-from mypy.plugin import AttributeContext, ClassDefContext, DynamicClassDefContext, MethodContext
+from mypy.plugin import AttributeContext, ClassDefContext, DynamicClassDefContext
+from mypy.plugins.common import add_method_to_class
from mypy.semanal import SemanticAnalyzer
from mypy.semanal_shared import has_placeholder
from mypy.subtypes import find_member
@@ -407,15 +408,20 @@ def create_manager_info_from_from_queryset_call(
return new_manager_info
-def create_manager_class(
- api: SemanticAnalyzer, base_manager_info: TypeInfo, name: str, line: int, with_unique_name: bool
-) -> TypeInfo:
+def _base_manager_instance(base_manager_info: TypeInfo) -> Instance:
base_manager_instance = fill_typevars(base_manager_info)
assert isinstance(base_manager_instance, Instance)
# If any of the type vars are undefined we need to defer. This is handled by the caller
if any(has_placeholder(type_var) for type_var in base_manager_info.defn.type_vars):
raise helpers.IncompleteDefnException
+ return base_manager_instance
+
+
+def create_manager_class(
+ api: SemanticAnalyzer, base_manager_info: TypeInfo, name: str, line: int, with_unique_name: bool
+) -> TypeInfo:
+ base_manager_instance = _base_manager_instance(base_manager_info)
if with_unique_name:
manager_info = helpers.add_new_class_for_module(
@@ -482,43 +488,26 @@ def populate_manager_from_queryset(manager_info: TypeInfo, queryset_info: TypeIn
)
-def create_new_manager_class_from_as_manager_method(ctx: DynamicClassDefContext) -> None:
- """
- Insert a new manager class node for a
-
- ```
- <manager name> = <QuerySet>.as_manager()
- ```
- """
+def add_as_manager_to_queryset_class(ctx: ClassDefContext) -> None:
semanal_api = helpers.get_semanal_api(ctx)
- # Don't redeclare the manager class if we've already defined it.
- manager_node = semanal_api.lookup_current_scope(ctx.name)
- if manager_node and manager_node.type is not None:
- # This is just a deferral run where our work is already finished
- return
- manager_sym = semanal_api.lookup_fully_qualified_or_none(fullnames.MANAGER_CLASS_FULLNAME)
- assert manager_sym is not None
- manager_base = manager_sym.node
- if manager_base is None:
+ def _defer() -> None:
if not semanal_api.final_iteration:
semanal_api.defer()
- return
-
- assert isinstance(manager_base, TypeInfo)
- callee = ctx.call.callee
- assert isinstance(callee, MemberExpr)
- assert isinstance(callee.expr, RefExpr)
-
- queryset_info = callee.expr.node
+ queryset_info = semanal_api.type
if queryset_info is None:
- if not semanal_api.final_iteration:
- semanal_api.defer()
+ return _defer()
+
+ # either a manual `as_manager` definition or this is a deferral pass
+ if "as_manager" in queryset_info.names:
return
- assert isinstance(queryset_info, TypeInfo)
+ manager_sym = semanal_api.lookup_fully_qualified_or_none(fullnames.MANAGER_CLASS_FULLNAME)
+ if manager_sym is None or not isinstance(manager_sym.node, TypeInfo):
+ return _defer()
+ manager_base = manager_sym.node
manager_class_name = manager_base.name + "From" + queryset_info.name
current_module = semanal_api.modules[semanal_api.cur_mod_id]
existing_sym = current_module.names.get(manager_class_name)
@@ -537,7 +526,7 @@ def create_new_manager_class_from_as_manager_method(ctx: DynamicClassDefContext)
api=semanal_api,
base_manager_info=manager_base,
name=manager_class_name,
- line=ctx.call.line,
+ line=queryset_info.line,
with_unique_name=True,
)
except helpers.IncompleteDefnException:
@@ -555,34 +544,22 @@ def create_new_manager_class_from_as_manager_method(ctx: DynamicClassDefContext)
queryset_info.metadata["django_as_manager_names"][semanal_api.cur_mod_id] = new_manager_info.name
# Add the new manager to the current module
- added = semanal_api.add_symbol_table_node(
- # We'll use `new_manager_info.name` instead of `manager_class_name` here
- # to handle possible name collisions, as it's unique.
- new_manager_info.name,
+ # We'll use `new_manager_info.name` instead of `manager_class_name` here
+ # to handle possible name collisions, as it's unique.
+ added = semanal_api.modules[semanal_api.cur_mod_id].names[new_manager_info.name] = (
# Note that the generated manager type is always inserted at module level
- SymbolTableNode(GDEF, new_manager_info, plugin_generated=True),
+ SymbolTableNode(GDEF, new_manager_info, plugin_generated=True)
)
assert added
-
-def construct_as_manager_instance(ctx: MethodContext, *, info: TypeInfo) -> MypyType:
- api = helpers.get_typechecker_api(ctx)
- module = helpers.get_current_module(api)
- try:
- manager_name = info.metadata["django_as_manager_names"][module.fullname]
- except KeyError:
- return ctx.default_return_type
-
- manager_node = api.lookup(manager_name)
- if not isinstance(manager_node.node, TypeInfo):
- return ctx.default_return_type
-
- # Whenever `<QuerySet>.as_manager()` isn't called at class level, we want to ensure
- # that the variable is an instance of our generated manager. Instead of the return
- # value of `.as_manager()`. Though model argument is populated as `Any`.
- # `transformers.models.AddManagers` will populate a model's manager(s), when it
- # finds it on class level.
- return Instance(manager_node.node, [AnyType(TypeOfAny.from_omitted_generics)])
+ add_method_to_class(
+ semanal_api,
+ ctx.cls,
+ "as_manager",
+ args=[],
+ return_type=Instance(new_manager_info, [AnyType(TypeOfAny.from_omitted_generics)]),
+ is_classmethod=True,
+ )
def reparametrize_any_manager_hook(ctx: ClassDefContext) -> None: ended up hitting: $ ../django-stubs/venv/bin/pip uninstall django-stubs -qqy && ../django-stubs/venv/bin/pip install -qq --no-deps ../django-stubs/ && ../django-stubs/venv/bin/python -m mypy main.py --disable-error-code empty-body --show-traceback
myapp/models.py:10: error: Return type "ManagerFromMyQuerySet[Any]" of "as_manager" incompatible with return type "ManagerFromBaseQuerySet[Any]" in supertype "BaseQuerySet" [override]
main.py:2: note: Revealed type is "myapp.models.ManagerFromMyQuerySet[myapp.models.MyModel]"
Found 1 error in 1 file (checked 1 source file) so yeah let's review / merge this in its current state for now! |
…) (#22) Co-authored-by: Anthony Sottile <[email protected]>
an absolute ton of work went into enabling this -- but after this PR model types / create / update / etc. should mostly be typechecked whereas they were entirely unchecked (everything `Any`) prior to this. most of the core problem was that mypy and django-stubs did not understand our custom `BaseManager` and `BaseQuerySet` since there's a significant amount of django stuff that goes into making those classes "real" fix that involved these issues: - python/mypy#17402 (worked around) - typeddjango/django-stubs#2228 (the workaround) with that fixed, it exposed a handful issues in django-stubs itself: - typeddjango/django-stubs#2240 - typeddjango/django-stubs#2241 - typeddjango/django-stubs#2244 - typeddjango/django-stubs#2248 - typeddjango/django-stubs#2276 - typeddjango/django-stubs#2281 fixing all of those together exposed around 1k issues in sentry code itself which was fixed over a number of PRs: - #75186 - #75108 - #75149 - #75162 - #75150 - #75154 - #75158 - #75148 - #75157 - #75156 - #75152 - #75153 - #75151 - #75113 - #75112 - #75111 - #75110 - #75109 - #75013 - #74641 - #74640 - #73783 - #73787 - #73788 - #73793 - #73791 - #73786 - #73761 - #73742 - #73744 - #73602 - #73596 - #73595 - #72892 - #73589 - #73587 - #73581 - #73580 - #73213 - #73207 - #73206 - #73205 - #73202 - #73198 - #73121 - #73109 - #73073 - #73061 - getsentry/getsentry#14370 - #72965 - #72963 - #72967 - #72877 (eventually was able to remove this when upgrading to mypy 1.11) - #72948 - #72799 - #72898 - #72899 - #72896 - #72895 - #72903 - #72894 - #72897 - #72893 - #72889 - #72887 - #72888 - #72811 - #72872 - #72813 - #72815 - #72812 - #72808 - #72802 - #72807 - #72809 - #72810 - #72814 - #72816 - #72818 - #72806 - #72801 - #72797 - #72800 - #72798 - #72771 - #72718 - #72719 - #72710 - #72709 - #72706 - #72693 - #72641 - #72591 - #72635 mypy 1.11 also included some important improvements with typechecking `for model_cls in (M1, M2, M2): ...` which also exposed some problems in our code. unfortunately upgrading mypy involved fixing a lot of things as well: - #75020 - #74982 - #74976 - #74974 - #74972 - #74967 - #74954 - getsentry/getsentry#14739 - getsentry/getsentry#14734 - #74959 - #74958 - #74956 - #74953 - #74955 - #74952 - #74895 - #74892 - #74897 - #74896 - #74893 - #74880 - #74900 - #74902 - #74903 - #74904 - #74899 - #74894 - #74882 - #74881 - #74871 - #74870 - #74855 - #74856 - #74853 - #74857 - #74858 - #74807 - #74805 - #74803 - #74806 - #74798 - #74809 - #74801 - #74804 - #74800 - #74799 - #74725 - #74682 - #74677 - #74680 - #74683 - #74681 needless to say this is probably the largest stacked PR I've ever done -- and I'm pretty happy with how I was able to split this up into digestable chunks <!-- Describe your PR here. -->
an absolute ton of work went into enabling this -- but after this PR model types / create / update / etc. should mostly be typechecked whereas they were entirely unchecked (everything `Any`) prior to this. most of the core problem was that mypy and django-stubs did not understand our custom `BaseManager` and `BaseQuerySet` since there's a significant amount of django stuff that goes into making those classes "real" fix that involved these issues: - python/mypy#17402 (worked around) - typeddjango/django-stubs#2228 (the workaround) with that fixed, it exposed a handful issues in django-stubs itself: - typeddjango/django-stubs#2240 - typeddjango/django-stubs#2241 - typeddjango/django-stubs#2244 - typeddjango/django-stubs#2248 - typeddjango/django-stubs#2276 - typeddjango/django-stubs#2281 fixing all of those together exposed around 1k issues in sentry code itself which was fixed over a number of PRs: - #75186 - #75108 - #75149 - #75162 - #75150 - #75154 - #75158 - #75148 - #75157 - #75156 - #75152 - #75153 - #75151 - #75113 - #75112 - #75111 - #75110 - #75109 - #75013 - #74641 - #74640 - #73783 - #73787 - #73788 - #73793 - #73791 - #73786 - #73761 - #73742 - #73744 - #73602 - #73596 - #73595 - #72892 - #73589 - #73587 - #73581 - #73580 - #73213 - #73207 - #73206 - #73205 - #73202 - #73198 - #73121 - #73109 - #73073 - #73061 - getsentry/getsentry#14370 - #72965 - #72963 - #72967 - #72877 (eventually was able to remove this when upgrading to mypy 1.11) - #72948 - #72799 - #72898 - #72899 - #72896 - #72895 - #72903 - #72894 - #72897 - #72893 - #72889 - #72887 - #72888 - #72811 - #72872 - #72813 - #72815 - #72812 - #72808 - #72802 - #72807 - #72809 - #72810 - #72814 - #72816 - #72818 - #72806 - #72801 - #72797 - #72800 - #72798 - #72771 - #72718 - #72719 - #72710 - #72709 - #72706 - #72693 - #72641 - #72591 - #72635 mypy 1.11 also included some important improvements with typechecking `for model_cls in (M1, M2, M2): ...` which also exposed some problems in our code. unfortunately upgrading mypy involved fixing a lot of things as well: - #75020 - #74982 - #74976 - #74974 - #74972 - #74967 - #74954 - getsentry/getsentry#14739 - getsentry/getsentry#14734 - #74959 - #74958 - #74956 - #74953 - #74955 - #74952 - #74895 - #74892 - #74897 - #74896 - #74893 - #74880 - #74900 - #74902 - #74903 - #74904 - #74899 - #74894 - #74882 - #74881 - #74871 - #74870 - #74855 - #74856 - #74853 - #74857 - #74858 - #74807 - #74805 - #74803 - #74806 - #74798 - #74809 - #74801 - #74804 - #74800 - #74799 - #74725 - #74682 - #74677 - #74680 - #74683 - #74681 needless to say this is probably the largest stacked PR I've ever done -- and I'm pretty happy with how I was able to split this up into digestable chunks <!-- Describe your PR here. -->
new test was previously failing with: